[MAINT] Fixup github_repository_file#3175
Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
1337fdb to
aef6dcf
Compare
stevehipwell
left a comment
There was a problem hiding this comment.
I missed the fact that the branch is optional, I think this should be required in the next major version. As it's optional then the import ID should allow it to be not set.
|
So what I'm saying is the following changes are needed to fix this resource.
|
Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
stevehipwell
left a comment
There was a problem hiding this comment.
We can simplify the logic by always requiring a 2 part ID, with the second part blank for the default ID; e.g. my-repo/my-file.txt:.
Longer term the ID should be in the form <repo>:<file>:<branch>, but that's a problem for another day.
Always use 2 part ID for import. Use buildID for resource ID
|
@stevehipwell Done. But I wonder if I should just refactor the import ID now as well? Sure, it's technically a breaking change, but imports are "one-off" usually |
@deiga I think this makes sense. @nickfloyd thoughts? We should also add |
Use 3 part ID for import as well
Signed-off-by: Timo Sand <[email protected]>
This required changing all tests to need a mock of the GH API Signed-off-by: Timo Sand <[email protected]>
|
@stevehipwell Addressed your comments. And re-introduced the API Mock pattern here, since it's the only way to verify the migration. Would you still prefer that we discuss before we can add this pattern? |
Yes, I don't think this is the time to add this; but I think it's something we need to explore further. |
stevehipwell
left a comment
There was a problem hiding this comment.
I've added inline comments, but you'll also need to add an ID set to update as the ID needs to be updated if the repo name has changed.
Doesn't ForceNew run |
The diff function supports changing the repo name without forcing new, so we have to set the ID in update. |
|
@stevehipwell If the |
Signed-off-by: Timo Sand <[email protected]>
The resource ID uses repo name so we need to call |
|
@stevehipwell Doh! 🤦 |
Signed-off-by: Timo Sand <[email protected]>
…ete` Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
|
@stevehipwell I went and deprecated the whole And, could you update the title of this PR to something like "resource_github_repository_file: fix import format and add state migrations" |
stevehipwell
left a comment
There was a problem hiding this comment.
This is looking good @deiga, just a couple of things to look at.
Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
Signed-off-by: Timo Sand <[email protected]>
github_repository_filegithub_repository_file
* Improve docs of importing `github_repository_file` Signed-off-by: Timo Sand <[email protected]> * Add missing test for import Signed-off-by: Timo Sand <[email protected]> * Ensure we only set `branch` on import if it's not the default branch Signed-off-by: Timo Sand <[email protected]> * Update `branch` to `Computed` field Signed-off-by: Timo Sand <[email protected]> * Add state migration for missing branch field Signed-off-by: Timo Sand <[email protected]> * Add ID migration in the same bunch Signed-off-by: Timo Sand <[email protected]> * Fix ID parsing Signed-off-by: Timo Sand <[email protected]> * fixup! Fix ID parsing Always use 2 part ID for import. Use buildID for resource ID * fixup! fixup! Fix ID parsing Use 3 part ID for import as well * fixup! fixup! fixup! Add computed `repository_id` field to resource Signed-off-by: Timo Sand <[email protected]> * fixup! fixup! fixup! fixup! Add migration of the `repository_id` field. This required changing all tests to need a mock of the GH API Signed-off-by: Timo Sand <[email protected]> * Comment out test in evaluation Signed-off-by: Timo Sand <[email protected]> * fixup! Address review comments Signed-off-by: Timo Sand <[email protected]> * fixup! Address review comments Signed-off-by: Timo Sand <[email protected]> * fixup! Remove usage of autocreate_branch in `Read`, `Update` and `Delete` Signed-off-by: Timo Sand <[email protected]> * Mark `autocreate_branch` as deprecated Signed-off-by: Timo Sand <[email protected]> * fixup! Address review comments Signed-off-by: Timo Sand <[email protected]> * fixup! Change to use `UpdateFile` in `Update` Signed-off-by: Timo Sand <[email protected]> * fixup! Update docs with missing fields Signed-off-by: Timo Sand <[email protected]> --------- Signed-off-by: Timo Sand <[email protected]>

Resolves #3161
Before the change?
github_repository_fileAfter the change?
github_repository_filebranchrepository_idfield and enables repo renaming without resource recreationbranchon create or import if not specified<repo>/<file>to<repo>:<file>:<branch>to enable safer usage of multiple files from same repo but different branchesbranch,repository_idand newIDformatPull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!